👏

Mostly unrelated to this PR, but these 3 lines come up again and again when working with rdflib.js. I'd love to have an alternative API which takes a string and returns a graph.

Again probably not in scope for the PR, but I'm confused as to why we need a readFile declared here vs fs.readFile.

Yeah, unsure. I just ported it.

doWhilst?? haha!

Just a nitpick comment (don't need to address) but the indentation here through me off as to which expression the anonymous function was being passed off to.

I can address this, as there are some other points as well. What indentation would you use here?

It's definitely a matter of taste but this is how I generally like things: when breaking function arguments up into several lines, I like to keep the closing paren ) on its own line, indented to match the start of the line of the function call.

For example:

ldpContainer.addContainerStats(
  ldp, reqUri, filename, resourceGraph,
  err => err ? reject(err) : resolve()
)

I also see this a lot when using callback functions - breaking the line after the arguments to the callback:

ldpContainer.addContainerStats(ldp, reqUri, filename, resourceGraph, err =>
  err ? reject(err) : resolve()
)

We should really consider using a linter that goes beyond standard. There are still lots of variations in the code.

This particular instance can perhaps be solved with promisify (see below).

Yeah, fair point. A code formatter would be nice. I thought I saw you discussing prettier at some point in gitter. I'd be in favor of adding that, for no other reason than not having to think about style ever again :)

Won't have been me 😄 I'm in favor of a strict .eslintrc (and really dislike standard 😉).

I'm leaving this as-is, since other parts in the same file follow the same indentation.

That's fine. Consistency is more important :)

It seems like a node convention for pre-Promise API async functions to take callbacks which themselves take a possible error as the first argument and a successful value as the next argument. We could write a higher-order function to make these kinds of functions return promises:

const promisify = (fn) =>
  (...args) =>
    new Promise((resolve, reject) =>
      fn(...args, (err, success) =>
        err
          ? reject(err)
          : resolve(success)
      )
    )

We could do that—and there are a couple of libraries that expose such a function (starting with Node 8 even in core). I've used that in other projects.

I was hesitant to do this here and in other places of the Solid code, because callbacks are sometimes used inconsistently (they don't always have an error argument). So I could start using it here, but then it would perhaps be good to keep an eye on it in other places of the code as well. And eventually, we should migrate to an await/async style, I suppose.

So, what do you think, should we start the promisify practice here?
Perhaps we should consider switching to async/await (with build step) for node-solid-server v5? Or already start it here?

Good points!

I was hesitant to do this here and in other places of the Solid code, because callbacks are sometimes used inconsistently (they don't always have an error argument).

Oof yeah that complicates things!

And eventually, we should migrate to an await/async style, I suppose.

Sure thing. Though async/await is syntactic sugar on top of Promises, so I think that "promisifying" our interfaces goes hand-in-hand with async/await-ifying them :)

I like your point that this change is more of an interface redesign than simply "promisifying" the existing APIs. How about we simply leave things as they are here, and I'll create a ticket for making the async APIs more consistent.

👍 I'll make the other changes and merge.

(created issue #557)

You can replace these four lines with:

nock('https://example.org')
  .get('/404').reply(404)
  .get('/401').reply(401)
  .get('/500').reply(500)
  .get('/200').reply(200) 

Thanks, will do.